fix(acp): pace targeted overflow recovery on consumer capacity - #7325

Review verdict: approve

Reviewed base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6 through exact live head 8000636f3073167c5a5107bb179c7d91160f1729, integrating both assigned systems and adversarial lanes. No author-actionable defect remains.

Targeted recovery is integrated into the production socket-owner select loop without blocking reads, commands, ping, or shutdown. It waits cancellation-safely for half-queue capacity, then rechecks receiver state, cooldown, shared quota, and active intent; fairness selects the least-recently-attempted eligible membership/channel intent. Failed writes retain the recovery cursor, successful REQ writes retire it at the existing boundary, unsubscribe clears associated recovery state, and reconnect preserves the established min(last_seen, dropped) restoration behavior. Production-seam tests prove replayed event delivery to the consumer rather than merely REQ emission.

Exact-head evidence: cargo test -p buzz-acp passed 916 unit tests plus 9 integration tests; git diff --check passed with a clean exact-head worktree. At submission, the live head remained exact, completed lint/security/DCO/cross-compile/platform checks were green, and no completed required check had failed. Remaining running unit, Desktop, relay-artifact, image, and other matrix jobs are owned by CI.

Author action: none.

Confidence gaps / verification owner: neither lane ran sustained real-relay + real-child loss/load or mutation-tested every regression. The production owner and real loopback WebSocket seams are covered, but relay retention/proxy behavior and downstream prompt completion remain reviewer/tooling follow-up. Catch-up remains intentionally best-effort after successful REQ write rather than exactly-once delivery, as documented.

— :bot: Jude’s code review agent

Team verdict: APPROVE at exact head 8000636f3073167c5a5107bb179c7d91160f1729 (base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6).

No author-actionable material defect was found across the systems/integration and adversarial product review lanes.

The review traced the production socket-owner path end to end: overflow preserves the oldest recovery cursor while releasing the dropped event from bounded transport dedup; capacity readiness participates in the existing responsive select!; recovery rechecks receiver state, half-queue headroom, cooldown, and shared quota; least-recently-attempted eligible channel or membership work is selected; failed writes retain recovery state; only a successful REQ write retires it; and unsubscribe/reconnect clean up or reconstruct the corresponding state. The changed regressions exercise the real background task and loopback WebSocket seam, including recurring headroom, fairness, cancellation, failed or stalled writes, reconnect, ping/shutdown responsiveness, and delivery of the replayed event to the consumer.

Exact-head evidence:

• . ./bin/activate-hermit && cargo test -p buzz-acp: 916 library tests and 9 integration tests passed; 0 failed (plus clean main/doc suites).
• git diff --check b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6...8000636f3073167c5a5107bb179c7d91160f1729: passed.
• Clean reviewer worktrees; local/remote PR head equality rechecked at lane close.
• Live integration state at submission still reports the same base/head and mergeable; Rust lint, security, DCO, Linux cross-compile results, and completed relevant checks are green. Remaining matrix/build jobs are still in progress.

Residual confidence gap: the reviewers did not run a sustained real-relay + real ACP-child stress cohort. The production-seam tests prove replay through the socket owner to the consumer, but not relay retention/history-limit behavior, proxy replay, or downstream ACP prompt completion. This is a reviewer/tooling follow-up, not author rework; the README accurately preserves the best-effort boundary after REQ write.

Author action: none.

Changed-head team verdict: APPROVE at exact head 6e398931169a6d64bb4e13a835fbb1d85ac144cb (base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6; prior approved head 8000636f3073167c5a5107bb179c7d91160f1729).

No author-actionable material defect was found.

The exact changed-head delta is one documentation-only commit: crates/buzz-acp/README.md is +4/-38; the production/test source tree, package manifest, and lockfile relevant to this change are byte-identical to the prior approved head. The condensed README contract remains accurate: affected-subscription replay is consumer-capacity and quota gated, paced no more often than every five seconds, depends on relay history and consumer progress, and does not guarantee complete delivery.

Both review lanes revalidated the unchanged production behavior: overflow preserves the oldest cursor while releasing undelivered IDs from bounded transport dedup; recovery readiness remains one arm of the responsive socket-owner select!; select-local capacity permits are dropped before dispatch and capacity is rechecked; least-recently-attempted eligible channel or membership work is selected; failed writes retain the cursor while successful REQ writes retire it; unsubscribe/access denial clears associated state; and reconnect resumes from min(last_seen, dropped). Existing production-seam regressions prove the replayed event reaches the consumer by exact ID and cover recurring pressure/fairness, stale EOSE, failed/stalled writes, headroom wake/cancellation, reconnect, unsubscribe, ping, shutdown, and transport loss.

Exact-head evidence:

• cargo test -p buzz-acp under pinned Hermit: 916 library tests + 9 integration tests passed; 0 failed; main/doc suites clean.
• git diff --check b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6...6e398931169a6d64bb4e13a835fbb1d85ac144cb: passed.
• Remote/local exact-head equality and clean worktrees rechecked after testing; merge-base equals the supplied base; all three commits carry DCO sign-offs.
• Live integration state at submission still reports this exact base/head as mergeable: 20 checks passed, 11 pending, 0 failed. Completed ACP-relevant Rust lint, security, DCO, Linux cross-compile, and image-build checks are green.

Residual confidence gap: sustained real-relay + real ACP-child pressure and mutation testing remain unrun. Relay retention/proxy behavior and downstream prompt completion therefore remain best-effort boundaries already documented by the PR. This is reviewer/tooling follow-up, not author rework.

Author action: none.

:bot: Jude’s code review agent — APPROVE at exact head 6e398931169a6d64bb4e13a835fbb1d85ac144cb against base b1f6b7ef770dddbb7f33c9f5861c379a47bca1d6.

No unresolved author-actionable defect remains after independent systems/integration and product/adversarial review.

The changed-head delta is documentation-only: the ACP production and test trees are byte-identical to the previously approved head. The condensed README remains accurate about targeted affected-subscription replay, capacity/quota gating, pacing, relay-history and consumer-progress dependency, and the absence of complete-delivery guarantees.

The integrated review revalidated socket-owner responsiveness, cancellation-safe capacity reservation, least-recently-attempted fairness, cursor retention on failed writes and retirement after successful REQ writes, unsubscribe cleanup, reconnect recovery, dedup handling, and production-seam tests that prove the lost event reaches the consumer rather than merely proving REQ emission.

Exact-head evidence: cargo test -p buzz-acp passed 916 library + 9 integration tests, with main/doc suites clean; git diff --check, merge-base, DCO, policy audit, exact local/remote SHA, and clean-tree checks passed. Completed Rust lint, security, Linux cross-compile, Windows Desktop, image, and release checks were green.

Confidence gaps, not author rework: sustained real-relay + real ACP-child pressure and mutation depth were not run. Several unrelated matrix/build checks remain in progress with no observed failure. Author action: none unless a pending required gate produces a PR-caused failure. Verification owner: CI for exact-head gate completion; reviewer/tooling for optional sustained live stress.

Any new head invalidates this approval.

:bot: Jude’s code review agent — APPROVE at exact head 183984d575e6ac1531ce92345f6178f7a32402d3 against base 218633b8fd6ee41aee8eb18ba9806e8d90694751.

No concrete defects found.

The changed-head review re-established that the recovery implementation is unchanged from the previously reviewed patch: commits 2–3 are identical, while commit 1 differs only around signed-event verification already introduced by the moved base. The recovery files and README are byte/patch identical. Verification now precedes routing and all cursor/dedup/queue mutation, so invalid events cannot contaminate replay state.

The integrated systems and adversarial traces found the intended contracts preserved:

• overflow removes only the undelivered event from bounded dedup and retains the oldest loss cursor;
• targeted replay preserves the existing filter and stable subscription ID, rewinds with the existing skew, and selects one least-recently-attempted eligible intent;
• capacity readiness uses a race-free half-capacity reservation local to the socket owner's select!; permits are dropped before competing socket/command work proceeds;
• blocked or failed writes retain the cursor, unsubscribe clears recovery state, and reconnect/rate-limit retry ownership remains separate;
• stale/current EOSE cannot incorrectly retire pending loss;
• tests cover capacity flapping, fairness, partial-permit cancellation, blocked writes, reconnect, unsubscribe/re-add, transport loss, ping, and shutdown;
• delivery is proved rather than merely REQ emission: the socket-owner test receives the exact lost event ID after replay (recovery_wake_tests.rs:121-137), while the pressure test receives exactly the 64 dropped IDs and then a subsequent live event (recovery_tests.rs:39-94).

Exact-head evidence:

• cargo test -p buzz-acp: 930 library + 9 integration tests passed; package targets and doc tests passed.
• cargo clippy -p buzz-acp --all-targets -- -D warnings, cargo fmt --all -- --check, and git diff --check passed.
• Local HEAD before/after validation was exact and the tree clean; fresh remote PR head/base remained exact and mergeable.
• Completed CI included Rust lint, security, Semgrep, zizmor, DCO, Linux cross-compiles, Windows Desktop build, and Desktop release candidate with no failures at review time.

Non-blocking confidence gaps: repository-wide exact-head CI is still running, and this review did not add sustained retained-history relay + real ACP-child pressure or broad mutation testing. Existing production-socket-owner/real-WebSocket tests prove harness-queue delivery, not arbitrary relay retention or downstream prompt completion; the README accurately documents that complete delivery remains dependent on relay history and consumer progress.

Author action: none.
Verification owner: CI for pending exact-head gates; reviewer/tooling for optional sustained real-relay/ACP-child stress and broader mutation depth.

Any new head invalidates this approval.

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: 218633b8fd6ee41aee8eb18ba9806e8d90694751..183984d575e6ac1531ce92345f6178f7a32402d3 (exact head 183984d575e6ac1531ce92345f6178f7a32402d3)
Risk: high — ACP relay overflow recovery changes socket-owner scheduling, queue-capacity coordination, replay cursors, and message continuity under pressure.

Behavior/contracts traced: socket-owner select lifecycle; cancellation-safe half-queue capacity wake; channel/membership fairness; shared quota ownership; failed/blocked write retry; cursor retirement; unsubscribe/reconnect cleanup; bounded dedup; signed-event verification introduced by the moved base; production-owner delivery tests.

Findings: no unresolved author-actionable defect.

The rebase was reviewed rather than waved through. Range-diff from prior approved head 6e398931169a6d64bb4e13a835fbb1d85ac144cb shows commits 2–3 unchanged. Commit 1 differs only where current base inserted signed-event verification and its tests; there is no changed production recovery hunk. Verification now precedes routing, replay cursor, dedup, and queue mutation, so invalid events cannot poison recovery state.

The integrated systems and product/adversarial lanes agree:

• Recovery readiness remains a tokio::select! arm beside socket reads, commands, ping, and shutdown; capacity permits are select-local and dropped before dispatch (crates/buzz-acp/src/relay.rs:1910-1914, recovery.rs:79-104).
• One least-recently-attempted eligible channel or membership intent is attempted after receiver, half-capacity, cooldown, and shared quota checks (recovery.rs:16-59,107-131). Failed writes retain the oldest loss cursor; successful REQ writes retire it at the documented best-effort boundary.
• Overflow removes only the undelivered event ID from bounded transport dedup and preserves the oldest dropped timestamp; unsubscribe/reconnect state transitions remain bounded and explicit (relay.rs:1295-1308,2237-2340).
• Tests prove delivery, not merely REQ emission: the production socket-owner seam returns the lost event on the stable subscription and asserts its exact event ID (recovery_wake_tests.rs:121-137); the pressure case proves replayed unique loss plus subsequent live delivery (recovery_tests.rs:39-94).

Author action: none.
Verification owner: CI for the 11 exact-head jobs still pending at submission; reviewer/tooling for optional sustained real-relay + real ACP-child stress and mutation depth.

Validation:

• PASS — pinned Hermit RUST_TEST_THREADS=4 cargo test -p buzz-acp: 930 library + 9 integration tests passed, 0 failed; main/doc targets clean. HEAD before/after was exact and tree remained clean.
• PASS — git diff --check 218633b...HEAD.
• PASS — independent range-diff/base-composition review.
• Live GitHub snapshot at submission: authenticated reviewer jedwards27, author loganj; 18 checks passed, 11 pending, 0 failed.

Manual/native evidence: not applicable; this is a headless ACP recovery path. Tests use the production socket owner and real WebSocket boundary.

Residual risk: no sustained retained-history relay plus real ACP-child pressure run was performed, so arbitrary relay retention/proxy behavior and downstream prompt completion remain unproven. The README accurately retains the best-effort/no-complete-delivery boundary. This is a reviewer/tooling confidence gap, not author rework.